-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ability to filter alerts by string parameters #92036
Ability to filter alerts by string parameters #92036
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
const keys = key.split('.'); | ||
for (let i = 0; i < keys.length; i++) { | ||
const path = `properties.${keys.slice(0, i + 1).join('.properties.')}`; | ||
if (get(indexMappings, path)?.type === 'flattened') { | ||
return true; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elastic/kibana-core this PR is ready for review. I had to add this piece of code and a few early returns to support flattened
type fields in alerting's use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I'm curious now if this can solve our "internal tags" requirement. I think it will require the per-type alert UIs to pre-fill these some new "hidden" params (hidden from the UI). For some reason I didn't realize you could access the individual fields in a flattened type in a query, I thought it just jammed all the text for all keys into a single field (which makes me curious how it's implemented)!
const response = await supertest.get( | ||
`${getUrlPrefix( | ||
Spaces.space1.id | ||
)}/api/alerts/_find?filter=alert.attributes.params.strValue:"my b"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's feeling like we're leaking Alerting internal implementation here...
I'm ok with that, as it feels like the overhead of trying to abstract the attributes
away is probably not worth it.... but I. would like this to have explicit documentation added (asciidocs/dev docs) as I don't think it's intuitive :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmmorris Agreed, I always felt like that with the KQL filter. Though, the enforcement of attributes
is made by the saved objects service so I figured we'd pass it through as we do with other fields.
Under filter
docs (https://www.elastic.co/guide/en/kibana/master/alerts-api-find.html) the params
would work the same as any other field but I think it makes sense to document that params
are analyzed as flattened and point to those docs so users understand the limitations of these fields.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense.
Putting myself in the mindset of a developer using our APIs - if the API explains what params
is and leads me to an API that explains how to query against it - that's fine.
Do we actually explain that the Alerts are stored as SOs? 🤔
I'm not sure the SO Api makes it clear how to query attributes though 🤔 I'll take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs:
(Optional, string) A KQL string that you filter with an attribute from your saved object. It should look like savedObjectType.attributes.title: “myTitle”. However, If you used a direct attribute of a saved object, such as updatedAt, you will have to define your filter, for example, savedObjectType.updatedAt > 2018-12-22.
I think the only extra mention we need is that they're flattened
and have filtering limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'd just like this to add some docs as I don't think using this is intuitive considering it relies on Alerting/SOs internal implementation
@elasticmachine merge upstream |
Yeah, it works pretty well which is why I didn't understand why there would be a use case for "internal tags" if we had this. I think we'll be able to close the issue in favour of this and ask for use-cases if ever this doesn't solve it. (one less thing to maintain). |
@@ -49,6 +49,8 @@ Retrieve a paginated set of alerts based on condition. | |||
NOTE: As alerts change in {kib}, the results on each page of the response also | |||
change. Use the find API for traditional paginated results, but avoid using it to export large amounts of data. | |||
|
|||
NOTE: Alert `params` are stored as {ref}/flattened.html[flattened] and analyzed as `keyword`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gchaps let me know what you think, I wanted to state that filtering on the params
field has limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've merged this PR, but I will create a follow-up if you would like changes to this line.
💚 Build Succeeded
Metrics [docs]Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: cc @mikecote |
* Initial commit * Update comment * Return early * Add docs Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* master: Ability to filter alerts by string parameters (elastic#92036) [APM] Fix for flaky correlations API test (elastic#91673) (elastic#92094) [Enterprise Search] Migrate shared role mapping components (elastic#91723) [file_upload] move ml Importer classes to file_upload plugin (elastic#91559) [Discover] Always show the "hide missing fields" toggle (elastic#91889) v2 migrations should exit process on corrupt saved object document (elastic#91465) [ML] Data Frame Analytics exploration page: filters improvements (elastic#91748) [ML] Data Frame Analytics: Improved error handling for scatterplot matrix. (elastic#91993) [coverage] speed up merging results of functional tests (elastic#92111) Adds a Reason indicator to the onClose handler in AddAlert and EditAlert (elastic#92149)
Resolves #92010
In this PR, I'm allowing alert parameters to be filtered on string values.